Support payments for less than the total MPP value#4373
Support payments for less than the total MPP value#4373TheBlueMatt wants to merge 9 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @valentinewallace as a reviewer! |
a4b00d4 to
e8d40e1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4373 +/- ##
==========================================
- Coverage 86.02% 86.01% -0.02%
==========================================
Files 156 156
Lines 103046 103169 +123
Branches 103046 103169 +123
==========================================
+ Hits 88646 88739 +93
- Misses 11888 11910 +22
- Partials 2512 2520 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
93228cf to
b6b973f
Compare
|
Opportunistically tagging this 0.3 since its rather important for some of our users (and its important for my wallet project!) |
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
| if let Some(proc_macro2::TokenTree::Group(group)) = ty_tokens { | ||
| let first_token = group.stream().into_iter().next(); | ||
| if let Some(proc_macro2::TokenTree::Ident(ident)) = first_token { | ||
| if is_init && ident == "legacy" { |
There was a problem hiding this comment.
Is this also suppose to check for "custom"? Without doing so, I'm unable to use this for a legacy field where it needs to be read in order to update another field.
error[E0560]: struct `FundingTxInput` has no field named `_sequence`
--> lightning/src/ln/funding.rs:120:6
|
120 | (3, _sequence, (custom, Sequence,
| ^^^^^^^^^ `FundingTxInput` does not have this field
|
= note: all struct fields are already assigned
For more information about this error, try `rustc --explain E0560`.
error: could not compile `lightning` (lib) due to previous error
This is what I tried in #4290.
diff --git a/lightning/src/ln/funding.rs b/lightning/src/ln/funding.rs
index 3b7c0bedb..2de763118 100644
--- a/lightning/src/ln/funding.rs
+++ b/lightning/src/ln/funding.rs
@@ -117,7 +117,19 @@ pub struct FundingTxInput {
impl_writeable_tlv_based!(FundingTxInput, {
(1, utxo, required),
- (3, _sequence, (legacy, Sequence, |input: &FundingTxInput| Some(input.utxo.sequence))),
+ (3, _sequence, (custom, Sequence,
+ |read_val: Option<Sequence>| {
+ if let Some(sequence) = read_val {
+ // Utxo contains sequence now, so update it if the value read here differs since
+ // this indicates Utxo::sequence was read with the default_value
+ let utxo: &mut Utxo = utxo.0.as_mut().expect("utxo is required");
+ if utxo.sequence != sequence {
+ utxo.sequence = sequence;
+ }
+ }
+ Ok(read_val)
+ },
+ |input: &FundingTxInput| Some(input.utxo.sequence))),
(5, prevtx, required),
});
There was a problem hiding this comment.
Hmm, no, grrrrrrr. I was using custom for the "we have a field we need to initialize, but we need to do so via some special logic" rather than what you really needed which is "we have removed this field and want to have some logic when reading its old TLV". I think what we actually need is a read method to legacy that works like the custom case but is ignored for field-initialization cases.
There was a problem hiding this comment.
What about checking if the field started with an underscore? Too risky / non-obvious?
There was a problem hiding this comment.
Yea, seems weird. legacy was kinda all about "item removed" whereas custom is about "item needs special handling". IMO it makes sense to have a method called on legacy after its read.
valentinewallace
left a comment
There was a problem hiding this comment.
Needs rebase, getting comments out so GH doesn't eat them
| } | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
There's a bunch of other parameters in PaymentParameters that we could be checking here. The method would get quite long though. Did you go over the options and decide not to verify them here?
There was a problem hiding this comment.
Ah I was actually just doing the things that are in RouteParameters, I hadn't considered the PaymentParameters fields. I can do some validation of those too.
b6b973f to
2abd54b
Compare
At various points we've been stuck in our TLV read/write variants but just want to break out and write some damn code to initialize a field and some more code to decide what to write for a TLV. We added the write-side part of this with the `legacy` TLV read/write variant, but its useful to also be able to specify a function which is called on the read side. Here we add a `custom` TLV read/write variant which calls a method both on read and write to either decide what to write or to map a read value (if any) to the final field.
When `OutboundPayments` calls the provided `Router` to fetch a `Route` it passes a `RouteParameters` with a specific max-fee. Here we validate that the `Route` returned sticks to the limits provided, and also that it meets the MPP rules of not having any single MPP part which can be removed while still meeting the desired payment amount.
2abd54b to
3d72f76
Compare
|
Rebased and addressed feedback. Note that I noticed the CLTV delta stuff for trampoline is a bit wrong so that's not checked here. |
3d72f76 to
1199749
Compare
In some uses of LDK we need the ability to send HTLCs for only a portion of some larger MPP payment. This allows payers to make single payments which spend funds from multiple wallets, which may be important for ecash wallets holding funds in multiple mints or graduated wallets which hold funds across a trusted wallet and a self-custodial wallet. In order to allow for this, we need to separate the concept of the payment amount from the onion MPP amount. Here we start this process by adding a `total_mpp_amount_msat` field to `RecipientOnionFields` (which is the appropriate place for a field describing something in the recipient onion). We currently always assert that it is equal to the existing fields, but will relax this in the coming commit(s). We also start including a payment preimage on probe attempts, which appears to have been the intent of the code, but which did not work correctly. The bulk of the test updates were done by Claude.
In some uses of LDK we need the ability to send HTLCs for only a portion of some larger MPP payment. This allows payers to make single payments which spend funds from multiple wallets, which may be important for ecash wallets holding funds in multiple mints or graduated wallets which hold funds across a trusted wallet and a self-custodial wallet. In the previous commit we added a new field to `RecipientOnionFields` to describe the total value of an MPP payment. Here we start using this field when building onions, dropping existing arguments to onion-building methods.
In some uses of LDK we need the ability to send HTLCs for only a portion of some larger MPP payment. This allows payers to make single payments which spend funds from multiple wallets, which may be important for ecash wallets holding funds in multiple mints or graduated wallets which hold funds across a trusted wallet and a self-custodial wallet. In the previous commits we moved the total-MPP-value we set in onions from being manually passed through onion-building to passing it via `RecipientOnionFields`. This introduced a subtle bug, though - payments which are retried will get a fresh `RecipientOnionFields` built from the data in `PendingOutboundPayment::Retryable`, losing any custom total-MPP-value settings and causing retries to fail. Here we fix this by storing the total-MPP-value directly in `PendingOutboundPayment::Retryable`.
In some uses of LDK we need the ability to send HTLCs for only a portion of some larger MPP payment. This allows payers to make single payments which spend funds from multiple wallets, which may be important for ecash wallets holding funds in multiple mints or graduated wallets which hold funds across a trusted wallet and a self-custodial wallet. In the previous few commits we added support for making these kinds of payments when using the payment methods which explicitly accepted a `RecipientOnionFields`. Here we also add support for such payments made via the `pay_for_bolt11_invoice` method, utilizing the new `OptionalBolt11PaymentParams` to hide the parameter from most calls. Test mostly by Claude
1199749 to
db6aa28
Compare
This adds support for it both in the more manual
send_paymentflow as well aspay_for_bolt11_invoice. Adding support for BOLT 12 is left for a followup. cc @benthecarman